Fix: Only try to remove deprecated plugins from array expressions #91
Conversation
For now only plugins instantiated in an array can be removed since this is safe. All other usages are considered not safe and thus won't be modified. - Display a message about the current support Closes #83
- Pass the source to the transform function - This allows displaying code snippets in error messages
| @@ -37,7 +37,7 @@ function transform(source, transforms, options) { | |||
| quote: 'single' | |||
| }, options); | |||
| transforms = transforms || Object.keys(transformations).map(k => transformations[k]); | |||
| transforms.forEach(f => f(jscodeshift, ast)); | |||
| transforms.forEach(f => f(jscodeshift, ast, source)); | |||
There was a problem hiding this comment.
What is the reason for adding source as an option?
There was a problem hiding this comment.
In order to print the code block: https://github.com/webpack/webpack-cli/pull/91/files#diff-33fa12ea113366fa6b41cce3aeb9bf6dR29
|
Updated description with a screenshot. |
|
Looks good LGTM on my end. Sending to @pksjce to final review. |
- Don't use `console.error` to fix colors. - Use chalk instead to make output more prettier. - Better error message with an example of the expected usage.
| ) { | ||
| // Check how many plugins are defined and | ||
| // if there is only last plugin left remove `plugins: []` node | ||
| if (path.parent.value.elements.length === 1) { |
There was a problem hiding this comment.
Good point but since I'm checking it's an array before, I think it's not pretty safe. Thoughts?
There was a problem hiding this comment.
Yeah. I meant you could replace all of this with safeTraverse.
const value = safeTraverse(path, ['parent', 'value']);
if (utils.isType(value, 'ArrayExpression')) {
const len = safeTraverse(value, ['elements', 'length']);
if (len && len === 1) {
...
} else
|
Yes, it is suppose to go out before merging with #92 |
| // Check how many plugins are defined and | ||
| // if there is only last plugin left remove `plugins: []` node | ||
| if (path.parent.value.elements.length === 1) { | ||
| j(path.parent.parent).remove(); |
There was a problem hiding this comment.
Is this a valid way of removing ast nodes? I did not know at all. I usually come down from the parent and then modify it to remove any node. :(
There was a problem hiding this comment.
Not sure what you mean by "valid way" but this works exactly as if I would come from the top of the tree.
|
@pksjce ouch you already merged it without waiting for my updates? |
|
Oh sorry! I misunderstood.
On Tue, Mar 21, 2017 at 4:08 PM Andrey Okonetchnikov < ***@***.***> wrote:
@pksjce <https://github.com/pksjce> ouch you already merged it without
waiting for my updates?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#91 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAZd9DKvLF9myrXfMNoyA-uR_8JJTfQGks5rn6i8gaJpZM4Mhtyr>
.
--
Regards,
Pavithra.K
|
What kind of change does this PR introduce?
bugfix
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
N/A
Summary
It fixes #83 and shows a nice error message for unsupported cases:
Does this PR introduce a breaking change?
Nope
Other information
For now, only plugins instantiated in an array can be removed by deprecated plugins transform since this is safe.